Skip to content

Conversation

@Pearl1594
Copy link
Contributor

@Pearl1594 Pearl1594 commented Dec 4, 2024

Description

Fixes: #10041
This PR provides Proper explanation for the global setting to avoid ambiguity

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • build/CI
  • test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

@codecov
Copy link

codecov bot commented Dec 4, 2024

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 16.00%. Comparing base (4f3e8e8) to head (d2b2419).
Report is 37 commits behind head on 4.20.

Files with missing lines Patch % Lines
...n/java/com/cloud/resource/ResourceManagerImpl.java 0.00% 2 Missing ⚠️
...java/com/cloud/agent/manager/AgentManagerImpl.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               4.20   #10042      +/-   ##
============================================
+ Coverage     15.99%   16.00%   +0.01%     
- Complexity    13076    13081       +5     
============================================
  Files          5649     5649              
  Lines        495612   495978     +366     
  Branches      59995    60134     +139     
============================================
+ Hits          79249    79364     +115     
- Misses       407514   407769     +255     
+ Partials       8849     8845       -4     
Flag Coverage Δ
uitests 4.03% <ø> (+0.02%) ⬆️
unittests 16.83% <0.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Pearl1594 Pearl1594 force-pushed the rename-ambiguous-globalsetting branch from 18cfe56 to 425fc95 Compare December 4, 2024 16:37
@Pearl1594 Pearl1594 changed the base branch from 4.19 to 4.20 December 4, 2024 16:37
@Pearl1594
Copy link
Contributor Author

@blueorangutan package

@blueorangutan
Copy link

@Pearl1594 a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 4, 2024

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 11714

@DaanHoogland DaanHoogland linked an issue Dec 5, 2024 that may be closed by this pull request
@DaanHoogland DaanHoogland added this to the 4.20.1 milestone Dec 5, 2024
Copy link
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks mostly good, needs testing

Copy link
Contributor

@shwstppr shwstppr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Pearl1594 I think if we just do the following change then it should be good enough. In my opinion, global config is largely okay.

@github-actions
Copy link

github-actions bot commented Jan 6, 2025

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

@Pearl1594
Copy link
Contributor Author

@nvazquez Apologies for the confusion, I had put the wrong name in the PR description - the global setting name has been renamed to kvm.host.auto.enable.disable. I've updated the schema file and fixed merge conflicts

@Pearl1594 Pearl1594 changed the title [WIP] Rename ambiguous global setting: enable.kvm.host.auto.enable.disable Rename ambiguous global setting: enable.kvm.host.auto.enable.disable Feb 12, 2025
@Pearl1594 Pearl1594 marked this pull request as ready for review February 12, 2025 15:43
@Pearl1594 Pearl1594 requested a review from shwstppr February 13, 2025 11:33
Copy link
Contributor

@nvazquez nvazquez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Pearl1594 LGTM - just a few non-blocking comments to keep the changes to minimal

Copy link
Contributor

@shwstppr shwstppr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor suggestions, otherwise code lgtm

@Pearl1594
Copy link
Contributor Author

@blueorangutan package

@blueorangutan
Copy link

@Pearl1594 a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12448

@Pearl1594
Copy link
Contributor Author

@blueorangutan test

@blueorangutan
Copy link

@Pearl1594 a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

@blueorangutan
Copy link

[SF] Trillian test result (tid-12412)
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 53507 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr10042-t12412-kvm-ol8.zip
Smoke tests completed. 139 look OK, 2 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File
test_09_connectivity_between_network_and_vpc_tier Failure 60.30 test_ipv4_routing.py
test_11_isolated_network_with_dynamic_routed_mode Error 2.32 test_ipv4_routing.py
test_12_vpc_and_tier_with_dynamic_routed_mode Error 3.42 test_ipv4_routing.py
test_12_vpc_and_tier_with_dynamic_routed_mode Error 3.42 test_ipv4_routing.py
test_06_purge_expunged_vm_background_task Failure 401.41 test_purge_expunged_vms.py

@Pearl1594
Copy link
Contributor Author

@blueorangutan package

@blueorangutan
Copy link

@Pearl1594 a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

Copy link
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clgtm

@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
C Maintainability Rating on New Code (required ≥ B)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12504

@Pearl1594
Copy link
Contributor Author

@blueorangutan test

@blueorangutan
Copy link

@Pearl1594 a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

@blueorangutan
Copy link

[SF] Trillian test result (tid-12541)
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 55107 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr10042-t12541-kvm-ol8.zip
Smoke tests completed. 140 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File
test_06_purge_expunged_vm_background_task Failure 385.22 test_purge_expunged_vms.py

@DaanHoogland DaanHoogland merged commit 3aabedd into 4.20 Mar 4, 2025
47 of 50 checks passed
@DaanHoogland DaanHoogland deleted the rename-ambiguous-globalsetting branch March 4, 2025 14:07
@Pearl1594 Pearl1594 mentioned this pull request Mar 5, 2025
@Pearl1594 Pearl1594 moved this to Done in ACS 4.20.1 Mar 17, 2025
dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Jun 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Confusing dialog text

5 participants